Fix fill values in all Omega fields#428
Conversation
7fc03ea to
8fc6203
Compare
8fc3284 to
6625253
Compare
|
It looks to me like it should be possible to automatically deduce the right fill value from the array type when calling |
6625253 to
8cc604f
Compare
|
@mwarusz, great idea! I'll see if I can incorporate that quickly into this PR. |
|
@xylar Here are some suggestions on how to do it cleanly in C++ that you might find useful. You could define the fill values in // Primary template intentionally undefined.
template <typename T>
constexpr T FillValue;
template <>
constexpr I4 FillValue<I4> = -2147483647; ///< NC_FILL_INT
template<>
constexpr I8 FillValue<I8> = -9223372036854775806LL; ///< NC_FILL_INT64
...Then if The main benefit of doing it this way is that you don't have to write a long chain of |
8cc604f to
147144f
Compare
|
@mwarusz, I like that. I'll implement it and run some tests, then I'll ask you to review once I've got it working. |
5fb3358 to
29d8f99
Compare
|
@mwarusz and @alicebarthel, I got optimistic about nearly being done and assigned you to review. But I still have some kinks to work out. I'll take this out of draft mode once it's working (hopefully tomorrow) and ping you again for a review. |
fba980b to
079b462
Compare
079b462 to
0804a22
Compare
TestingI tested this after rebasing onto I am using the polaris branch E3SM-Project/polaris#603 for testing to make sure that the higher-order horizontal advection fix still works with fill values. CTest unit tests:
Polaris
|
|
@mwarusz and @alicebarthel, I know you may both be busy with work related to the All-hands but I would appreciate a review on this. I had started to implement a proper treatment of the surface pressure -- initially just reading it in from the initial state if it is present. But that work ran into the problem of needing to define a fill value for SurfacePressure. Thus, I decided to work on this first. I will base my SurfacePressure work on this branch but would appreciate getting it in before too long. At the same time, I recognize that these changes touch a fair bit of code, so a good review may require some time. I am happy to explain my thinking, particularly regarding the handling of fields on edges. |
|
@brian-oneill, I also added you as a reviewer. The changes here are extensive enough that it feels like one more pair of eyes on the code would be a good idea. |
0804a22 to
fce8ffa
Compare
This merge adds 5 constexpr fill value constants (FillValueI4, FillValueI8, FillValueR4, FillValueR8, FillValueReal) in namespace OMEGA, wrapping PIO_FILL_* from SCORPIO.
This merge adds private fillWithValue<T>() template helper (using if constexpr on T::value_type) and a call to it at the end of attachData<T>(). Arrays are now auto-filled with the declared fill value at attach time.
This merge removes all local fill value declarations and replaces references with the centralized constants.
In some cases we definitely don't want to overwrite fields we attach with fill values. Fix 2 such cases in Tracers and OceanState
We don't want fill values at every boundary edge becausee these are also valid. Instead, we want zeros except for edges between two inactive cells (e.g. both adjacent cells are below bathymetry)
Rather than assuming that the NormalVelocity read from an initial condition has correct masking, we ensure that it has fill values for inactive layers, zeros at boundary edges (both adjacent to land and to bathymetry), and leave its values unchanged for active, non-boundary layers.
We need to ensure that it is zero at active boundary edges.
Co-authored-by: Maciej Waruszewski <mwarusz@igf.fuw.edu.pl>
Add Test 5 and Test 6 to FillValueTest.cpp covering the cell and vertex layer-mask methods added in 27c02f5. Each test fills a synthetic field with a sentinel value, applies the mask, and verifies the resulting zones: cells expect FillValueReal in inactive layers and the sentinel in active layers; vertices expect the 3-zone pattern (fill / 0 / sentinel) matching the existing edge-mask test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document applyCellLayerMask and applyVertexLayerMask alongside the edge helpers in Section 4.5, and describe the OceanState::applyLayerMasks driver called from ocnInit. Correct stale facts in Section 5: the test lives at test/ocn/FillValueTest.cpp, uses direct comparison with Error accumulation, and now includes cell and vertex mask cases (5.5, 5.6); remove the unimplemented NetCDF attribute test. Update Requirement 2.7 for the expanded cell/vertex coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a Layer masking subsection to the VertCoord devGuide describing zeroEdgeField, applyEdgeLayerMask, applyCellLayerMask, and applyVertexLayerMask, their zones, the inclusive active-layer convention, and the applyLayerMasks driver. Add a userGuide note explaining that inactive layers carry the fill value and boundary layers carry 0 in output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
58bd787 to
b569502
Compare
Latest TestingI tested this including the fix in #456 CTest unit tests:
Polaris
|
|
My hope is that this can be merged right after #456 since the two should ideally be tested together. |
|
@sbrus89 and @alicebarthel, I believe this and #456 are waiting on your final approval before getting merged. Do you think it will be possible to get them approved and merged today? They're holding up a lot of other work. |
Fix fill-value handling across Omega so inactive parts of arrays hold well-defined, NetCDF-standard fill values instead of uninitialized memory or inconsistent local sentinels, initialize the Kokkos arrays backing fields at attach time, and mask cell, edge, and vertex state fields to their active layers after initialization. Based on the companion PR that fixes biharmonic velocity mixing at boundary vertices (peeled out of this branch in #456).
Motivation
Omega fields declared their fill values locally in each module with inconsistent values (
-9.99e30,-9.99E+30,-999, etc.) that did not match the NetCDF-C standard. More critically, the Kokkos arrays backing those fields were never explicitly initialized: inactive ocean layers (outside a column's active range) held uninitialized memory rather than a well-defined sentinel, so output files contained garbage rather than a recognizable fill value. Switching from zero-initialization to fill-value initialization also exposed a latent class of bugs where edge and boundary-layer fields assumed zero at mesh boundaries, producing corrupted tendencies when those layers were no longer zero by default. This change standardizes the fill-value constants, auto-fills arrays at attach time, and masks cell, edge, and vertex state fields to their valid layer ranges (with the correct per-geometry boundary treatment) after IC/restart read.Summary
Fix fill-value handling across Omega:
components/omega/src/base/FillValues.hwith centralizedconstexprfill value constants (FillValueI4,FillValueI8,FillValueR4,FillValueR8,FillValueReal) that exactly match the NetCDF-CNC_FILL_*/ SCORPIOPIO_FILL_*values. A type-indexed variable templateFillValue<T>provides the primary interface.FillValueargument fromField::create()(and fromTracers::define()/TracerDefs.inc). The fill value is now deduced automatically from the array's element type atattachData()time.Field::attachData<T>(): a privatefillWithValue<T>()helper callsKokkos::deep_copy(InDataArray, FillValue<ValType>)and records the value inFieldMeta["_FillValue"](the CF/NetCDF standard attribute). An optionalFillOnAttach = falseargument covers the cases whereattachData()is called only to re-point a field at an already-computed array (time-level updates inOceanStateandTracers, and the IO time-coordinate attach/assign ordering).VertCoord:zeroEdgeFieldandapplyEdgeLayerMaskfor edge fields (three-zone), andapplyCellLayerMaskandapplyVertexLayerMaskfor cell and vertex fields (two-zone). Cell and vertex fields have no zeroed boundary zone: a cell layer is simply active or inactive, and a boundary vertex with one or more active surrounding cells holds valid, generally non-zero data.OceanState::applyLayerMasks(TimeLevel), a single entry point that appliesapplyEdgeLayerMasktoNormalVelocityandapplyCellLayerMasktoPseudoThickness,Temperature, andSalinity; it is called fromOceanInitafter the IC/restart read.NormalVelocityis additionally masked in theOceanStateconstructor and after eachupdateVelocityByTendstep in the time stepper to keep the three-zone invariant throughout the run.zeroEdgeFieldonNormalVelocityTendandVelocityDel2Aux.Del2Edgeat the start of each tendency computation, so boundary layers show0rather thanFillValueRealas required for flux-type edge fields.MinLayerCell,MaxLayerCell,BottomGeomDepth,RefPseudoThickness, andVertCoordMovementWeightsfrom theInitialVertCoordstream, with robust fill-value-count detection so a missing or unreadable variable falls back to a sensible default (e.g.MinLayerCellat the surface,MaxLayerCellatNVertLayers) instead of propagating fill values into the vertical-coordinate setup.VertAdv::computeVerticalPseudoVelocity(DivHU): clamp the inner edge loop toK <= MaxLayerEdgeTop(JEdge)to avoid reading fill values from edges shallower than the owning cell.FillValueTestCTest (8 MPI tasks) with six subtests: fill-constant equality withNC_FILL_*,attachDataauto-fill, inactive cell-layer fill afterVertCoordinitialization, the three-zoneNormalVelocityinvariant after IC read andapplyEdgeLayerMask, the two-zoneapplyCellLayerMaskpattern, and the two-zoneapplyVertexLayerMaskpattern.components/omega/doc/design/FillValues.mdand update the Field, Tracer, and VertCoord developer/user guides to describe fill values and layer masking.Layer masking approach
Fields span layers
[0, NVertLayers), but the valid extent of each cell, edge, or vertex is set by the active layers of the neighboring cells. The masking helpers enforce this per geometry.Edge fields use four per-edge layer indices (stored in
VertCoord) that define three zones:MinLayerEdgeTopMinLayerEdgeBotMaxLayerEdgeTopMaxLayerEdgeBotZone 1 — fully inactive (
K < MinLayerEdgeToporK > MaxLayerEdgeBot): both neighboring cells are inactive at this layer. These layers holdFillValueRealand must never participate in any computation.Zone 2 — boundary layer (
MinLayerEdgeTop <= K < MinLayerEdgeBotorMaxLayerEdgeTop < K <= MaxLayerEdgeBot): exactly one neighbor is active. For flux-type edge fields (NormalVelocity,NormalVelocityTend,Del2Edge) the physically correct value is0because no flux crosses a solid boundary. For thickness-interpolated fields (MeanPseudoThickEdge,FluxPseudoThickEdge) the value is genuinely undefined and retainsFillValueReal.Zone 3 — active (
MinLayerEdgeBot <= K <= MaxLayerEdgeTop): both neighbors are active; the field carries a normally computed value.VertCoord::zeroEdgeFieldsets zones 1+2 to0(used at the start of each tendency step for flux fields that are recomputed each step, where theattachDatafill cannot be relied on).VertCoord::applyEdgeLayerMaskimposes the full three-zone pattern (zone 1 → fill, zone 2 → 0, zone 3 → unchanged) and is applied after IC/restart read and after each velocity tendency update.Cell and vertex fields use a simpler two-zone pattern, because there is no flux-through-boundary condition to enforce.
applyCellLayerMasksets layers outside[MinLayerCell, MaxLayerCell]toFillValueRealand leaves the active range unchanged.applyVertexLayerMasksets layers outside[MinLayerVertexTop, MaxLayerVertexBot]toFillValueRealand leaves the active range unchanged; unlike edges there is no zeroed boundary zone, since a boundary vertex adjacent to at least one active cell holds valid, generally non-zero data.Checklist
Testingwith the following:have been run on and indicate that are all passing.
has passed, using the Polaris
e3sm_submodules/Omegabaseline-pfor both the baseline (Polarise3sm_submodules/Omega) and the PR buildFixes #347
Fixes #409